fix: make gate counting functions less confusing and avoid estimations #9046
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
lucasxia01
requested review from
jeanmon,
IlyasRidhuan and
fcarreiro
as code owners
fcarreiro
reviewed
Oct 7, 2024
There was a problem hiding this comment.
Could you change the .hbs files that are used to generate these? Otherwise they will be rolled back next time we generate our code
lucasxia01
changed the title
lucasxia01
commented
Oct 8, 2024
| @@ -142,23 +142,15 @@ UltraWithKeccakVerifier UltraComposer::create_ultra_with_keccak_verifier(Circuit | |||
| return output_state; | |||
| } | |||
|
|
|||
| size_t UltraComposer::compute_dyadic_circuit_size(CircuitBuilder& circuit) | |||
There was a problem hiding this comment.
redundant, could just call the builder functions that do the exact same thing
lucasxia01
commented
Oct 8, 2024
lucasxia01
commented
Oct 8, 2024
| // Construct Honk proof | ||
| Prover prover{ builder }; | ||
| init_bn254_crs(prover.proving_key->proving_key.circuit_size); |
There was a problem hiding this comment.
core change: avoiding a needless builder computation just to get an incorrect srs size...
lucasxia01
commented
Oct 8, 2024
lucasxia01
commented
Oct 8, 2024
| @@ -701,7 +697,8 @@ template <typename Builder = UltraCircuitBuilder> void gateCount(const std::stri | |||
| for (auto constraint_system : constraint_systems) { | |||
| auto builder = acir_format::create_circuit<Builder>( | |||
| constraint_system, 0, {}, honk_recursion, std::make_shared<bb::ECCOpQueue>(), true); | |||
| builder.finalize_circuit(); | |||
| builder.finalize_circuit(/*ensure_nonzero=*/true); | |||
There was a problem hiding this comment.
core change: fix thats needed to properly add in the ensure nonzero gates
lucasxia01
commented
Oct 8, 2024
| void AcirComposer::create_circuit(acir_format::AcirFormat& constraint_system, | ||
| WitnessVector const& witness, | ||
| bool collect_gates_per_opcode) | ||
| void AcirComposer::create_finalized_circuit(acir_format::AcirFormat& constraint_system, |
There was a problem hiding this comment.
core change: finalizing the circuit here
lucasxia01
commented
Oct 8, 2024
| @@ -32,13 +32,23 @@ class AcirComposer { | |||
| bool verify_proof(std::vector<uint8_t> const& proof); | |||
|
|
|||
| std::string get_solidity_verifier(); | |||
| size_t get_total_circuit_size() { return builder_.get_total_circuit_size(); }; | |||
| size_t get_dyadic_circuit_size() { return builder_.get_circuit_subgroup_size(builder_.get_total_circuit_size()); }; | |||
| size_t get_finalized_total_circuit_size() { return builder_.get_finalized_total_circuit_size(); }; | |||
There was a problem hiding this comment.
two new functions that give a finalized size (and check that the circuit is actually finalized)
lucasxia01
commented
Oct 8, 2024
barretenberg/cpp/src/barretenberg/dsl/acir_proofs/acir_composer.hpp
Outdated
Show resolved
Hide resolved
lucasxia01
commented
Oct 8, 2024
| auto total_with_extra_gates = builder.get_total_circuit_size() + num_extra_gates; | ||
| *total = htonl((uint32_t)total_with_extra_gates); | ||
| *subgroup = htonl((uint32_t)builder.get_circuit_subgroup_size(builder.get_total_circuit_size())); | ||
| builder.finalize_circuit(/*ensure_nonzero=*/true); |
There was a problem hiding this comment.
we just overestimate if we ever have to pick. This function is used by plonk and honk, so we just go with adding more gates
lucasxia01
commented
Oct 8, 2024
lucasxia01
commented
Oct 8, 2024
barretenberg/cpp/src/barretenberg/smt_verification/circuit/standard_circuit.hpp
Outdated
Show resolved
Hide resolved
ledwards2225
approved these changes
Oct 8, 2024
barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/ultra_honk/decider_proving_key.cpp
Outdated
Show resolved
Hide resolved
barretenberg/cpp/src/barretenberg/ultra_honk/decider_proving_key.hpp
Outdated
Show resolved
Hide resolved
…col/aztec-packages into lx/fix-finalize-circuit-calls
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes unnecessary gate counting in Honk flows in main.cpp and instead inits the SRS based on the actual finalized gate count taken from the Prover.
Renames get_num_gates to get_estimated_num_finalized_gates, renames a few other functions to add "estimated" to their names to reflect their actual functionality and avoid misuse.
Replace estimating functions used in main.cpp and in c_bind.cpp with functions that return the actual finalized gate count.
Fixes a bug in an earlier PR #9042, which forgot the ensure_nonzero = true argument to finalize_circuit(), and undercounted the number of gates in the circuit, leading a smaller than needed SRS.